🐛 make create verb a fixed namespaced collection verb for preflight checks#2587
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR fixes Boxcutter applier preflight RBAC validation so it correctly checks for namespace-scoped CREATE permissions, preventing installs from passing preflight but failing later when creating namespaced resources (e.g., ServiceAccounts).
Changes:
- Configure Boxcutter’s
RBACPreAuthorizerwithWithNamespacedCollectionVerbs("create")under thePreflightPermissionsfeature gate.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kuiwang02
changed the title
kuiwang02
changed the title
kuiwang02
changed the title
|
/cc @perdasilva |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2587 +/- ##
==========================================
+ Coverage 65.84% 68.86% +3.01%
==========================================
Files 137 139 +2
Lines 9560 9872 +312
==========================================
+ Hits 6295 6798 +503
+ Misses 2795 2557 -238
- Partials 470 517 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
please add e2e tests that assert the change.
@fgiudici Thanks! |
@pedjak I add e2e cases for the change and it passes. could you please review it again? Thanks |
|
/approve |
openshift-ci
bot
added
the
approved
|
@pedjak could you please review it again? thanks. |
|
/lgtm |
|
ping @pedjak |
| @@ -0,0 +1,68 @@ | |||
| --- | |||
There was a problem hiding this comment.
this file is not needed, because we execute the new test only with boxcutter runtime.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@pedjak you'll have to remove your "Changes Requested" for this PR to advance (even if it has an LGTM). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@perdasilva Following your feedback, this PR now uses a different approach: Instead of requiring each applier to explicitly call WithNamespacedCollectionVerbs("create"), we now define namespaceCollectionVerbs as a fixed variable (similar to objectVerbs) and initialize it by default in NewRBACPreAuthorizer. please help review it again. thanks |
kuiwang02
changed the title
| authorizer: newRBACAuthorizer(cl), | ||
| ruleResolver: newRBACRulesResolver(cl), | ||
| restMapper: cl.RESTMapper(), | ||
| namespacedCollectionVerbs: slices.Clone(namespacedCollectionVerbs), |
There was a problem hiding this comment.
let's remove this field from the struct and just reference namespacedCollectionVerbs directly like we do with objectVerbs. Wdyt?
There was a problem hiding this comment.
@perdasilva based on your comment, I make the change https://github.com/operator-framework/operator-controller/compare/3860ac5bf8ecbcf0207138793f3a9c74df9ce231..42bc39a00af0f6e6c11df5fa6e4a38b0ea2dac6f and the test is failing (before this change, the test is ok)
But I do not find root cause on why it is failing (I do not think the changed code cause it). so, I want to rerun test. but I do not know how to rerun it. could you please help me to rerun the test job? thanks
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/retest-required |
|
/approve |
|
/test experimental-e2e |
|
@kuiwang02: No presubmit jobs available for operator-framework/operator-controller@main DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fgiudici, pedjak, perdasilva, rashmigottipati, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 7145047
into
operator-framework:main
Summary
Makes the
createverb a fixed default for namespaced collection verbs in RBAC preflight checks, ensuring consistent permission validation across all appliers (Boxcutter and Helm).Problem
PR #2539 introduced configurable namespace-scoped collection verbs for preflight permission checks. However:
WithNamespacedCollectionVerbs("create")configurationOriginal issue: Only Boxcutter was affected because Helm had the correct configuration.
Approach Change (Per @perdasilva's Suggestion)
Following feedback from @perdasilva, this PR uses a broader approach instead of just adding the missing configuration to Boxcutter:
Instead of requiring each applier to explicitly call
WithNamespacedCollectionVerbs("create"),We now define
namespacedCollectionVerbsas a fixed variable (similar toobjectVerbs) and initialize it by default inNewRBACPreAuthorizer.Related Issues